Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retrofit repo class as context-man to cleanup global mman on repo-delete #555

Merged
merged 4 commits into from
Feb 25, 2017

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Dec 8, 2016

Some changes for this release:
See #553 for difficulties when working on Windows, where open file-handles actually "lock" the underlying files, and repos cannot be deleted.

More changes:

@codecov-io
Copy link

codecov-io commented Dec 8, 2016

Current coverage is 94.50% (diff: 92.00%)

No coverage report found for master at 0691441.

Powered by Codecov. Last update 0691441...f858c44

@Byron
Copy link
Member

Byron commented Dec 8, 2016

@ankostis Do you think I should wait till tomorrow until I cut the release to be able to include this one? Or would you prefer to just have this released separately at a later time?

@Byron Byron modified the milestones: v2.1.1 - Bugfixes, v2.1.2 - Bugfixes Dec 8, 2016
@Byron
Copy link
Member

Byron commented Dec 8, 2016

@nvie I just created a new release.
@ankostis I will follow this PR and make a release when needed. Thanks to the Makefile, that became easy and convenient (interestingly, I wasn't aware it existed anymore :)).

@ankostis
Copy link
Contributor Author

ankostis commented Dec 8, 2016

Sorry @Byron , I've just seen your message.

I'm not exactly sure what happened and you could not include my PR in 2.1.1.
I believe it ready to be merged.
is this right, or this PR relies on features from a different GitDB/smmap?

@Byron
Copy link
Member

Byron commented Dec 9, 2016

Even though the implementation seems to be in place, it would certainly be nice to include an example in the test_docs.py to show people how to use it. I for one wouldn't know myself actually, just because my experience with context managers is very limited.
Do you think you could add an example? I could then include it in the tutorial and write some prose around it :).

@ankostis
Copy link
Contributor Author

ankostis commented Feb 5, 2017

Apologies for not having the time,
but I really believe this little PR wroth to have it.

Too overloaded to fix the TCs in the next weeks :-(

@Byron Byron merged commit 0181a40 into gitpython-developers:master Feb 25, 2017
@Byron Byron removed the in progress label Feb 25, 2017
@Byron
Copy link
Member

Byron commented Feb 25, 2017

I think so too :)! Sorry for the incredible delays.

@ankostis ankostis deleted the cntxtmman branch June 18, 2018 15:45
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Dec 12, 2023
The gc module was already imported at module scope in
git/repo/base.py, since f1a82e4 (gitpython-developers#555). Importing the top-level
git module or any submodule of it runs that import statement.
Because the gc module is already imported, reimporting it is fast.

Imports that there is no specific reason to do locally should be at
module scope. Having them local decreased readability, in part
because of how black inserts a black line between them and
gc.collect() calls they are imported to allow.

An alternative to this change would be to remove the preexisting
top-level "import gc" (there is also another one in the test suite)
and replace it with a local import as well. I am unsure if that
would affect performance and, if so, whether the effect would be
good or bad, since the small delay of the import might potentially
be less desirable to an applicaion if it occurs while the work of
the application is already in progress.

If a gc.collect() call runs as a consequence of a finally block or
__del__ method being called during interpreter shutdown, then in
(very) rare cases the variable may have been set to None. But this
does not appear to have been the intent behind making the imports
local. More importantly, a local import should not be expected to
succeed, or the imported module usable, in such a situation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants